-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Raise TypeError in easy_install.CommandSpec.from_param
#4548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
TypeError in easy_install.CommandSpec.from_param
| def test_from_param_raises_expected_error(self) -> None: | ||
| """ | ||
| from_param should raise its own TypeError when the argument's type is unsupported | ||
| """ | ||
| try: | ||
| ei.CommandSpec.from_param(object()) # type: ignore[arg-type] # We want a type error here | ||
| except Exception as error: | ||
| assert type(error) is TypeError, error | ||
| assert ( | ||
| str(error) == "Argument has an unsupported type <class 'object'>" | ||
| ), error | ||
| else: | ||
| raise AssertionError( | ||
| "from_param did not raise any error for argument of unsupported type" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/pypa/setuptools/actions/runs/10307637126/job/28533318670?pr=4548#step:5:74
setuptools/tests/test_easy_install.py (85.7%): Missing lines 1347
Darn, coverage check includes the test themselves. Is there a better way to do a "expect to fail with specific error" test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it can be a bit overwhelming.
Is there a better way to do a "expect to fail with specific error" test ?
Have you tried pytest.raises?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I found as well. Looks a lot cleaner, thanks.
fab64a4 to
79d5665
Compare
9674f40 to
190462c
Compare
abravalheri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WE can merge this once we are preparing a new major release.
Thank you!
Didn't make it to 73 ^^" |
190462c to
fbc75bc
Compare
|
Sorry, I was not cutting that release :( Let's just wait two weeks and merge it anyway. |
|
It seems that there might be a release soon with changes in distutils... So it might be a good idea to merge this now. |
Summary of changes
Follow-up to #4505 (comment)
As mentioned in that comment, let's include this PR in a major update since, although an edge case, isn't backwards compatible.
Changed the type of error raised by
setuptools.command.easy_install.CommandSpec.from_paramon unsupported argument fromAttributeErrortoTypeError.Pull Request Checklist
newsfragments/.(See documentation for details)